Skip to content

jit: guard the residual-call dispatch table's covered width and drop its per-call allocations - #1006

Merged
youknowone merged 2 commits into
mainfrom
rewrite-tracer
Aug 3, 2026
Merged

jit: guard the residual-call dispatch table's covered width and drop its per-call allocations#1006
youknowone merged 2 commits into
mainfrom
rewrite-tracer

Conversation

@youknowone

@youknowone youknowone commented Aug 3, 2026

Copy link
Copy Markdown
Owner

Follow-up to #1002 (Codex P1 + two CodeRabbit findings on that PR).

#1002 replaced the bucket-keyed residual-call dispatch with a class-sequence
table, matching descr.py:574 / descr.py:604-605 create_call_stub
declaration order. Three loose ends were left on it.

1. The table's covered width was stated nowhere on the descr-build side

dispatch_classes_body! enumerates one extern "C" signature per ordered
argument-class sequence: every sequence up to 5 arguments, plus the
all-Int sequences on to MAX_HOST_CALL_ARITY. Nothing said so where
calldescrs are minted, so adding an f64 parameter to an already-wide helper
would surface only as the table's catch-all panic — at the first deopt that
dispatches that call, far from the edit that caused it.

MAX_FLOAT_CARRYING_CALL_ARITY = 5 now sits next to BhCallDescr, with a
debug_assert on all three of its constructors. Signatures past
MAX_HOST_CALL_ARITY are deliberately exempt: residual_call.rs already
declines to emit the call at that width, so no blackhole ever dispatches them
and asserting would turn an orderly decline into a debug panic.

Upstream needs no such bound — create_call_stub source-generates
FuncType(ARGS, RESULT) per calldescr at translation time, so every sequence
has a stub. Lifting it here means an ABI adapter for signatures known only at
run time; the catch-all's TODO records that convergence path.

2. Two heap allocations per residual call

collect_call_args returned two Vecs on a path the blackhole takes for every
residual call. It now fills a fixed CallArgs buffer sized
MAX_HOST_CALL_ARITY — the same bound the widest arm stops at, so a signature
that does not fit has no arm either. The wasm host-trampoline path keeps
collect_call_args_positional unchanged.

3. An undescribed argument slot silently became an integer

arg_classes_from_types read arg_types with .get(i) and mapped None to
ArgClass::Int — exactly the mis-placement #1002 exists to prevent, had the
undescribed slot been a Float. Both callers take the positional arguments and
the type list from the same calldescr, so that is now a debug_assert.

Tests

  • a float in the last covered slot dispatches ((i64,i64,i64,i64,f64));
  • one argument past that width is refused, not mis-placed;
  • collect_call_args refuses more arguments than the table covers.

Verification

  • cargo test -p majit-backend -p majit-metainterp -p majit-translate -p pyre-jit-trace — 0 failed. The debug build exercises debug_assert_dispatchable against the whole in-tree descr universe.
  • python3 pyre/check.py --backend dynasm,cranelift,wasm — ALL PASSED: dynasm 370/370, cranelift 370/370, wasm 366/366.

authored by Claude

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of integer, floating-point, reference, and void calls across supported execution backends.
    • Added validation to reject unsupported call signatures and prevent incorrect argument classification.
    • Fixed edge cases involving floating-point arguments in typed calls.
  • Performance

    • Reduced temporary allocations during call argument processing.
    • Added support and coverage for the widest supported floating-point call signatures.

…width

`dispatch_classes_body!` enumerates one `extern "C"` signature per ordered
argument-class sequence: every sequence up to 5 arguments, plus the all-Int
sequences on to MAX_HOST_CALL_ARITY. That float-carrying width was stated
nowhere on the descr-build side, so a wider float-bearing signature would
reach the table's catch-all panic only at the first deopt that dispatches it.

Add MAX_FLOAT_CARRYING_CALL_ARITY = 5 next to BhCallDescr and a
debug_assert on its three constructors. Signatures past MAX_HOST_CALL_ARITY
are exempt: residual_call.rs declines to emit the call at that width, so no
blackhole dispatches them.

Add call_stub tests for a float in the last covered slot and for the refusal
one argument past it.

Assisted-by: Claude
…typed-call type list covers every slot

`collect_call_args` returned two `Vec`s, allocated on every residual call the
blackhole dispatches. Return a fixed-size `CallArgs` buffer sized
MAX_HOST_CALL_ARITY — the bound the dispatch table's widest arm already stops
at — and update the dynasm, cranelift and shared `*_by_classes` call sites.
The wasm host-trampoline path keeps `collect_call_args_positional`.

`arg_classes_from_types` read `arg_types` with `.get(i)` and mapped a missing
slot to `ArgClass::Int`, which would put an undescribed Float argument in an
integer register. Both callers take the positional arguments and the type list
from the same calldescr, so debug_assert that the type list is no shorter.

Assisted-by: Claude
@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b0849cb9-0064-42de-b93a-3535632de319

📥 Commits

Reviewing files that changed from the base of the PR and between e07e525 and 5879f6f.

📒 Files selected for processing (5)
  • majit/majit-backend-cranelift/src/compiler.rs
  • majit/majit-backend-dynasm/src/runner.rs
  • majit/majit-backend/src/call_stub.rs
  • majit/majit-metainterp/src/pyjitpl/dispatch.rs
  • majit/majit-translate/src/codewriter/jitcode.rs

Walkthrough

collect_call_args now returns bounded CallArgs storage. Dispatch signatures enforce supported arity limits. Cranelift, DynASM, and typed dispatch paths use accessor methods for collected argument classes and values.

Changes

Residual-call dispatch

Layer / File(s) Summary
Bounded call-argument collection
majit/majit-backend/src/call_stub.rs
collect_call_args now fills fixed-size CallArgs storage and exposes class and value slices. Tests cover five-argument float-carrying calls and over-wide signatures.
Dispatch signature validation
majit/majit-translate/src/codewriter/jitcode.rs, majit/majit-metainterp/src/pyjitpl/dispatch.rs
Call descriptor construction validates dispatchable signatures. Typed calls assert that positional argument types are fully described.
Backend dispatch migration
majit/majit-backend/src/call_stub.rs, majit/majit-backend-cranelift/src/compiler.rs, majit/majit-backend-dynasm/src/runner.rs
Integer, reference, floating-point, and void dispatch paths consume CallArgs through classes() and args().

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested reviewers: lifthrasiir

Poem

A rabbit packs args in a buffer so neat,
Five floaty hops fit beneath its feet.
Call classes line up, values follow through,
Cranelift and DynASM know what to do.
Wide calls stop at the bounded gate—
Typed paths check their slots are straight.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the dispatch-width safeguards and removal of per-call allocations.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch rewrite-tracer

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5879f6fd89

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

return;
}
debug_assert!(
!arg_classes.contains('f') || arity <= MAX_FLOAT_CARRYING_CALL_ARITY,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Support wide float descriptors instead of rejecting them

In debug builds, any valid residual-call descriptor with 6–16 arguments and at least one f64 now panics during construction, even though it is within MAX_HOST_CALL_ARITY and compiled traces/wasm can place the signature. This replaces upstream's per-descriptor stub support with an artificial rejection; release builds merely defer the same unsupported signature to a deoptimization-time panic. Extend the native dispatcher or use the documented ABI-adapter approach rather than rejecting these descriptors.

AGENTS.md reference: AGENTS.md:L194-L196

Useful? React with 👍 / 👎.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

🤖 Codex parity review

Static analysis of this diff vs the local RPython/PyPy sources (commit 5879f6f).
Updated: 2026-08-03T10:33:52.353Z

Files in the reviewed diff
majit/majit-backend-cranelift/src/compiler.rs
majit/majit-backend-dynasm/src/runner.rs
majit/majit-backend/src/call_stub.rs
majit/majit-metainterp/src/pyjitpl/dispatch.rs
majit/majit-translate/src/codewriter/jitcode.rs

1. Regressions to PyPy parity introduced by this patch

  • majit/majit-translate/src/codewriter/jitcode.rs:962 ↔ rpython/jit/backend/llsupport/descr.py:647 — new debug-only rejection of any float-bearing call with 6–16 arguments. PyPy accepts every arg_classes sequence and generates its exact FuncType; this patch now panics while constructing the descriptor, earlier than main did.

2. Other mismatches introduced by this patch

None.

3. Pre-existing mismatches (already present before this patch)

  • majit/majit-backend/src/call_stub.rs:1065 ↔ rpython/jit/backend/llsupport/descr.py:598 — the hand-written dispatcher supports float-bearing signatures only through arity 5 and integer-only signatures only through arity 16; PyPy generates a per-descriptor stub for every argument sequence.

  • majit/majit-backend/src/call_stub.rs:1267 ↔ rpython/jit/backend/llsupport/descr.py:551SingleFloat ('S') calls panic, whereas PyPy converts with int2singlefloat and includes lltype.SingleFloat in the generated function signature.

  • majit/majit-metainterp/src/pyjitpl/dispatch.rs:8546 ↔ rpython/jit/backend/llsupport/descr.py:647 — in release builds, a positional argument not covered by arg_types is silently classified as an integer. PyPy derives one argument class from every declared ARGS entry; there is no equivalent untyped fallback.

4. Structural adaptations

  • majit/majit-backend/src/call_stub.rs:1124 ↔ rpython/jit/backend/llsupport/descr.py:598 — replacing transient Vecs with a fixed CallArgs stack buffer is a Rust-specific allocation-free representation. For signatures supported by the existing dispatcher, it preserves PyPy’s declaration-order bank consumption.

@youknowone
youknowone merged commit 26831cb into main Aug 3, 2026
19 checks passed
@youknowone
youknowone deleted the rewrite-tracer branch August 3, 2026 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant